@glneo so, if cache ops are called on regions that are not-cached, then they should become no-op by the lower-level API. I don't understand how separate option helps.

If the lower-level API is responsible for checking the cachability of the region I would be okay with that, but then we could just remove the WITH_CACHE option and always call the cache ops for all memory accesses, letting the lower-level API sort things out.

If the lower-level API is responsible for checking the cachability of the region I would be okay with that, but then we could just remove the WITH_CACHE option and always call the cache ops for all memory accesses, letting the lower-level API sort things out.

@glneo Not all the cores have cache hardware available. In that case it makes sense to not compile cache related APIs. But if Cache hardware is available and for one region if its already used then why not let it handle other regions. We can argue that it would make un-necessary call to the API, that can slow down fw execution a bit. But other than that, I don't understand how introducing these options solves actual bug. If DCACHE was enabled that should have resolved any cache related bug. I am just trying to understand how this patch is helping in your case.

We can argue that it would make un-necessary call to the API, that can slow down fw execution a bit.

I would agree with you that this is a micro-optimization that isn't an issue worth worrying about. So then why not remove the WITH_CACHE and make the just always make the calls? If the core doesn't have cache hardware then those calls to metal_cache_{flush,invalidate}() become NOPs and libmetal usually has them inline in headers so the compiler will just drop them in the first place.

So either compile-time removing of these calls matters, in which case removing them as needed as this PR does would matters. Or it doesn't matter and I can send an update removing WITH_CACHE completely.

We can argue that it would make un-necessary call to the API, that can slow down fw execution a bit.

I would agree with you that this is a micro-optimization that isn't an issue worth worrying about. So then why not remove the WITH_CACHE and make the just always make the calls? If the core doesn't have cache hardware then those calls to metal_cache_{flush,invalidate}() become NOPs and libmetal usually has them inline in headers so the compiler will just drop them in the first place.

So either compile-time removing of these calls matters, in which case removing them as needed as this PR does would matters. Or it doesn't matter and I can send an update removing WITH_CACHE completely.

That is not how the library design works. Expecting users to provide empty stubs for the hardware their platform does not have is not a standard practice. We can always make exceptions, but it is not the case here. Anyway, my question is still unanswered, i.e. what kind of problem this patch is solving? I am trying to understand the motivation behind this patch. Is it like do you see significant improvement in firmware execution time or any bug that is fixed with this patch?

@arnopo do you have any comment here?

@glneo I am not against this patch. At least it is still improving performance by avoiding redundant cache calls. I just want to understand your use case.

That is not how the library design works. Expecting users to provide empty stubs for the hardware their platform does not have is not a standard practice.

Sure it is, for instance libmetal always provides metal_cache_{flush,invalidate}(), and when you port that library to your platform you can either provide it if available or stub it out, but it is always an exposed function as part of the library.

At least it is still improving performance by avoiding redundant cache calls.

That's all I was trying to fix here, but honestly I'm now thinking it should be up to the lower layer to decide if a cache operation is needed or not. Having it a compile time decision here just makes things less portable without having to change around build flags. To go from one platform with caches to one without, we shouldn't have to mess with platform specific build flags. Everything should be handled by libmetal, that is the point of libmetal, to make this library more portable. All the machine/system/platform specifics should be hidden down in the libmetal layer IMHO.

That is not how the library design works. Expecting users to provide empty stubs for the hardware their platform does not have is not a standard practice.

Sure it is, for instance libmetal always provides metal_cache_{flush,invalidate}(), and when you port that library to your platform you can either provide it if available or stub it out, but it is always an exposed function as part of the library.

Nope. Let's understand `metal_cache_{flush,invalidate} calls. I see the libmetal implementation, it is like this:

metal_cache_flush() -> __metal_cache_flush() -> metal_machine_cache_flush.

metal_machine_cache_flush is an interface, and user is expected to provide its implementation only if it ends up using that interface. By default libmetal provides that implementation via template machine: https://github.com/OpenAMP/libmetal/blob/80ab6b0d506a0d8eb4f2b87926682ababbb804b3/lib/system/generic/template/sys.c#L43

I can always choose not to use the template machine, and build the libmetal for my own machine, or not pass any machine at all and provide machine specific interfaces via some other library or implement them in the application itself.

So, ideally if I build open-amp with WITH_DCACHE=off option then it should build.
Today, we get an error because in the libmetal, metal_cache_{flush,invalidate} is used in device.c file: https://github.com/OpenAMP/libmetal/blob/80ab6b0d506a0d8eb4f2b87926682ababbb804b3/lib/device.c#L137

I don't think metal_cache* calls belong to that device.c file, and user should explicitly flush invalidate cache after using those dma ops, if their platform has the cache hardware. I will create PR to remove those calls from device.c, so users don't have to provide empty stubs.

At least it is still improving performance by avoiding redundant cache calls.

That's all I was trying to fix here, but honestly I'm now thinking it should be up to the lower layer to decide if a cache operation is needed or not. Having it a compile time decision here just makes things less portable without having to change around build flags. To go from one platform with caches to one without, we shouldn't have to mess with platform specific build flags. Everything should be handled by libmetal, that is the point of libmetal, to make this library more portable. All the machine/system/platform specifics should be hidden down in the libmetal layer IMHO.

The libmetal library should provide abstraction for sure but, should not enforce abstraction on the users. That means, user shouldn't have to provide implementation of the calls they are not planning to use in their applications / libraries.

I'm feeling there is a very fundamental disagreement on the whole point of a library in general. A library provides a consistent and stable interface to perform some utility. The whole point of libmetal is to provide a portable interface for applications across different devices.

Check the very first sentence of the very first paragraph of the libmetal readme:

Libmetal provides common user APIs..

Now you are saying you will just decide not to implement some APIs, not even as a stub, you break the basic promise of libmetal.

That means, user shouldn't have to provide implementation of the calls they are not planning to use in their applications / libraries.

What are you talking about, users never have to provide implementations of these functions, we do, we provide libmetal to the users of our hardware. You and me are not users, we are vendors. And when we say we have an implementation of libmetal for our hardware our users expect us to provide the libmetal API, not just the parts we like. If some function isn't needed for some hardware (in this case if we don't have caches) then we make it a NOP, the compiler optimizes it out of the end application, no harm done. On the other hand if you don't provide even the stub and just break the API, well as you point out, even libmetal itself(!) doesn't build if the metal_cache_*() functions don't exist. How are all the users of libmetal going to deal with that.

Libmetal provides common user APIs..

They are interfaces, not the implementation in many cases.

If we wanted to enforce the stubs for metal_cache* functions, we wouldn't provide WITH_DCACHE option in the first place.
We removed these options because open-amp already has many configurations. So, converting from three to one made sense. Removing even WITH_DCACHE doesn't make sense at all.

Anyway, I don't think I need to answer here further due to fundamental misunderstanding.
I will let @arnopo decide on this patch.

Hi,

From my point of view:

• Any API used by, or potentially usable by, the OpenAMP library must be implemented in libmetal.
  • metal_irq_* APIs are not used by the OpenAMP library, but they may be used by applications, so they are optional.
  • metal_cache_* APIs can be used by OpenAMP, so libmetal must provide an implementation, whether it is a stub or not.

Concerning cache management here is a proposal:

• OpenAMP is not aware of a memory region’s cache capability.
• This information should be bettre handled at the application level, or in libmetal’s I/O layer.

If we want to move cache configuration from OpenAMP to libmetal while preserving code optimization:

• In OpenAMP, we could deprecate WITH_DCACHE and consider it enabled by default.
• In libmetal:

Option 1:

• Create new APIs to manage cache operations for buffers, vrings, and resource tables:
  • metal_cache_buffers_flush/invalidate
  • metal_cache_vring_flush/invalidate
  • metal_cache_rsc_tab_flush/invalidate
• Add CMake options to enable or disable each cache method. If disabled, the functions would be empty and generate no code.

Option 2:

• Use a single WITH_DCACHE option for all cache management.

I would favor option 2, as it seems simpler and reflects the fact that the OpenAMP library should remain agnostic. The OS should be able to manage memory cacheability; otherwise, this can be handled in the machine setup.

@glneo, @tnmysh: would it make sense for you?

I'd like to avoid adding new metal_cache_{buffers,vring,rsc}_flush/invalidate() functions, those would be very specific to the OpenAMP usage, but libmetal should remain generic and can be used by other projects.

Existing libmetal API should be good enough, implementation for a given hardware will fall into one of 3 buckets:

1. All regions are cached and so the current metal_cache_flush/invalidate() are sufficient and would always perform the requested operation.
2. No region is cached and so the current metal_cache_flush/invalidate() are sufficient and are just NOP stubs.
3. Some regions are cached and some are not, in this case the platform has two options:
   • Always perform the operation anyway, which for there would be a minuscule overhead which in theory could have been avoided
   • Check the region in question based on the memory address provided and perform the operation conditionally

For the 3rd case the overhead of checking the memory cacheability would probably take longer than just issuing the cache operation unconditionally and letting the hardware ignore it. As I think our (TI) implementation would be the only one currently using mixed region cacheability, and we are okay with the overhead, this isn't an issue. Other implementations can always handle it they way they prefer.

Should I update this PR to deprecate WITH_DCACHE and consider it enabled by default. Or close this PR and make a new one for that?

Hi,

From my point of view:

• Any API used by, or potentially usable by, the OpenAMP library must be implemented in libmetal.

  • metal_irq_* APIs are not used by the OpenAMP library, but they may be used by applications, so they are optional.
  • metal_cache_* APIs can be used by OpenAMP, so libmetal must provide an implementation, whether it is a stub or not.

@arnopo exactly, We decided not to use metal_cache* APIs for the open-amp library by giving user a choice via WITH_DCACHE option. Now if we remove that option, then we are taking away that choice from the user and enforcing use of those APIs even if the hardware doesn't support it.

I think we should discuss this on system-reference call and make a decision. I am against removing that option. I am okay with this patch.

Concerning cache management here is a proposal:

• OpenAMP is not aware of a memory region’s cache capability.
• This information should be bettre handled at the application level, or in libmetal’s I/O layer.

If we want to move cache configuration from OpenAMP to libmetal while preserving code optimization:

• In OpenAMP, we could deprecate WITH_DCACHE and consider it enabled by default.
• In libmetal:

Option 1:

• Create new APIs to manage cache operations for buffers, vrings, and resource tables:

  • metal_cache_buffers_flush/invalidate
  • metal_cache_vring_flush/invalidate
  • metal_cache_rsc_tab_flush/invalidate

• Add CMake options to enable or disable each cache method. If disabled, the functions would be empty and generate no code.

Option 2:

• Use a single WITH_DCACHE option for all cache management.

I would favor option 2, as it seems simpler and reflects the fact that the OpenAMP library should remain agnostic. The OS should be able to manage memory cacheability; otherwise, this can be handled in the machine setup.

@glneo, @tnmysh: would it make sense for you?

I do not want to move CACHE configuration option to the Libmetal. It's correct in the open-amp library. I just don't want to remove it.

I think we should discuss this on system-reference call and make a decision.

That's fine, I'll join tomorrow and we can chat this out. In general though discussions here are nice as they provide better tracking for folks who might not be able to make the calls or are finding this thread sometime in the future.

5/6/2026 OpenAMP system-reference call:
Got agreement that will put the option in cmake configuration for libmetal instead of in open-amp. Will have to go through deprecation process.